Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use "variable" kind for parameter completion #2061

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

filipw
Copy link
Member

@filipw filipw commented Jan 7, 2021

We used to do it this way until the switch to CompletionService. This PR restores the old mapping of parameter to variable.

Fixes #2060
Fixes dotnet/vscode-csharp#4314

@filipw
Copy link
Member Author

filipw commented Jan 7, 2021

cc @333fred

@333fred
Copy link
Contributor

333fred commented Jan 7, 2021

Interesting. This is copied directly from roslyn, https://github.com/dotnet/roslyn/blob/master/src/Features/LanguageServer/Protocol/Extensions/ProtocolConversions.cs#L57. @dibarbet, is Roslyn intentionally using value for parameters there, or is it an oversight?

@filipw
Copy link
Member Author

filipw commented Jan 7, 2021

yes I saw this too and got confused. but then tested in VS directly and saw there that locals and parameters use same glyph. is it remapped elsewhere?

@333fred
Copy link
Contributor

333fred commented Jan 7, 2021

yes I saw this too and got confused. but then tested in VS directly and saw there that locals and parameters use same glyph. is it remapped elsewhere?

That map is only used by the LSP, so you won't see it unless you're using codespaces.

@dibarbet
Copy link
Contributor

dibarbet commented Jan 7, 2021

@dibarbet, is Roslyn intentionally using value for parameters there, or is it an oversight?

good question, it very well could be an oversight, or VS just uses the same icon for both, @allisonchou do you happen to remember?

@allisonchou
Copy link
Contributor

good question, it very well could be an oversight, or VS just uses the same icon for both, @allisonchou do you happen to remember?

I'm not sure. 😦 It may have been an oversight. Looking at the file history, it looks like this mapping has been here since LSP was first tested out in Roslyn.

@333fred
Copy link
Contributor

333fred commented Jan 8, 2021

Filed dotnet/roslyn#50324 on Roslyn to investigate whether we're using the wrong value as well.

@filipw filipw merged commit e758077 into OmniSharp:master Jan 8, 2021
@filipw
Copy link
Member Author

filipw commented Jan 8, 2021

thanks!

@filipw filipw deleted the bugfix/icon branch January 8, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants